Add feature flag to remove use of "INSERT RETURNING" in NewOrderAndAuthzs #7739
Conversation
|
@aarongable, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values. |
|
@aarongable, this PR adds one or more new feature flags: InsertAuthzsIndividually. As such, this PR must be accompanied by a review of the Let's Encrypt CP/CPS to ensure that our behavior both before and after this flag is flipped is compliant with that document. Please conduct such a review, then add your findings to the PR description in a paragraph beginning with "CPS Compliance Review:". |
|
Benchmarks, where true/false indicates whether the new feature flag is enabled, and the integer represents how many new authzs were being inserted in a single batch. The results show that the flag-true codepath is statistically slower, by about 15% in the most extreme case: |
On my own machine, an Apple M2 Max, I'm seeing ~15% in the most extreme case as well. |
There was a problem hiding this comment.
Seems good. I'm surprised this shows only a 15% slowdown in worst case (100 authzs to create). I suspect that's because the mariadb instance is on the same machine and therefore has very low latency.
I think it's okay to land this as-is because it's behind a feature flag, but I suspect we'll see a much worse worst-case slowdown in realistic situations and wind up deciding to do the insertions somewhat in parallel (less than our current parallelism, because spiky connection counts, but more than 1).
We could also simulate slowdown a little more accurately by using latency distribution stats from prod.
aarongable
requested review from
a team and
jprenken
and removed request for
a team
| nil, | ||
| nil, | ||
| }) | ||
| err = tx.Insert(ctx, am) |
There was a problem hiding this comment.
One difference between this codepath and the current codepath is that the MultiInserter code below hardcodes nil for four fields: Attempted, AttemptedAt, ValidationRecord, and ValidationError. These fields should never be set in a NewOrderAndAuthzs request from the RA. Is it worth adding some extra lines of code to this new codepath to retain the same defense-in-depth as the old codepath has? Or are we happy abandoning that teensy bit of extra safety?
There was a problem hiding this comment.
I feel like either way is OK; defense in depth is great and I wouldn't say no to it, but this bad case feels impossible.
This is our only use of MariaDB's "INSERT ... RETURNING" syntax, which does not exist in MySQL and Vitess. Add a feature flag which removes our use of this feature, so that we can easily disable it and then re-enable it if it turns out to be too much of a performance hit.
Also add a benchmark showing that the serial-insertion approach is slower, but perhaps not debilitatingly so.
CPS Compliance Review: This feature flag does not risk noncompliance with our CP/CPS.
IN-10737 tracks the corresponding SRE changes to enable this flag.
Part of #7718